Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tombstoning #598

Merged
merged 49 commits into from
Sep 9, 2024
Merged

Tombstoning #598

merged 49 commits into from
Sep 9, 2024

Conversation

lukasjuhrich
Copy link
Collaborator

@lukasjuhrich lukasjuhrich commented Jan 28, 2023

  • Implement UnixTombstone with FKeys, indices, and check constraints
  • Generic tests
  • Test check constraint
  • Test trigger creating Tombstone
  • lifetime restriction: Add ON DELETE SET NULL to user→unix_account (since the unix_account now does not serve as tombstone anymore) and ensure user deletion causes unix_account deletion
  • Think about whether it makes sense to keep unix_accounts without a user (the ldap_exporter ignores them, anyway) (answer: automatically delete the unix account on user deletion, but technically allow them)
  • add migration for schema changes
  • proper schema documentation
  • vendor mermaid.js

@lukasjuhrich lukasjuhrich force-pushed the tombstoning branch 2 times, most recently from 87d99ed to bbb35f6 Compare July 6, 2024 17:46
pycroft/model/user.py Outdated Show resolved Hide resolved
pycroft/model/user.py Outdated Show resolved Hide resolved
@lukasjuhrich
Copy link
Collaborator Author

lukasjuhrich commented Jul 8, 2024

The PR currently implements things like this:

---
title: Tombstone Consistency
config:
  fontFamily: monospace
---
erDiagram
    User {
        int unix_account_id
        str login
        str login_hash "generated always as digest(login, 'sha512')"
    } 
    UnixAccount {
        int id
        int uid
    }
    UnixTombStone {
        int uid
        str login_hash
    }

    User |o--o| UnixAccount: "user_unix_account_id_fkey"
    User ||--o| UnixTombStone: "user_login_hash_fkey"
    UnixAccount ||--o| UnixTombStone: "unix_account_uid_fkey"

Loading

One new important change would be that even though unix accounts without corresponding user are allowed, deleting the User also deletes the UnixAccount. This makes sense, because there is no need to keep the unix account. In particular, removing the unix account helps us to find out which home directories we should archive (namely, the ones without a corresponding row in unix_account)

@lukasjuhrich
Copy link
Collaborator Author

Unfortunately, I cannot naturally create a cascade which deletes the UnixAccount if the user gets deleted – unless I flip the User←→UnixAccount dependency by adding a UnixAccount.user_id column. Any opinions on that @sebschrader @FestplattenSchnitzel?

@lukasjuhrich
Copy link
Collaborator Author

lukasjuhrich commented Jul 8, 2024

I cannot naturally create a cascade which deletes the UnixAccount if the user gets deleted – unless I […] [add] a UnixAccount.user_id column

Not true, I can just instruct sqlalchemy to issue the deletes by adding cascade="delete".
This is the much less intrusive option than the schema change.

This encompasses
- A generated column `User.login_hash`
- An FKey User→UnixTombstone
- An FKey UnixAccount→UnixTombstone
- Partial indices on the UnixTombstone table to ensure a composite
  nullable key with optional components, but with equality
- A trigger updating the user's tombstone if a UnixAccount is inserted
First, setting `echo=True` on the engine has the disadvantage that
logs are emitted internally _and_ to stdout, leading to double
reporting.

Furthermore, we now have a cascade of three levels
1. no verbosity (0): no logs emitted.
2. `-v` (1): statement logs are emitted, but not results – and only
   displayed by pytest on failure.
3. `-vv` (2): both statement logs and result rows are emitted, also on
   failure.
@lukasjuhrich lukasjuhrich force-pushed the tombstoning branch 2 times, most recently from 75e94f7 to 851c325 Compare July 8, 2024 12:46
This has been deprecated in v0.7 because `execute_if` already subsumes
this functionality;
see https://github.com/sqlalchemy/sqlalchemy/blob/f6198d9abf453182f4b111e0579a7a4ef1614e79/lib/sqlalchemy/sql/ddl.py#L314-L316.

We already use that on the event registration when `dialect` is set.
Some mis-attribution of things as `hybrid_property` caused the sphinx
lookup of the respective attributes on the class to fail.
Copy link
Member

@ibot3 ibot3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't do an in-depth review currently but the changes look good for me.

The first thing one should see is the actual test, not details
regarding the setup.

This follows the general suggestion to place the high-level functions on
the top, and the functions which are dependencies more towards the bottom.
@lukasjuhrich lukasjuhrich force-pushed the tombstoning branch 2 times, most recently from 772d387 to de80e90 Compare September 4, 2024 14:43
@lukasjuhrich
Copy link
Collaborator Author

@ibot3 FYI there was something I overlooked: testing that the "is login available" logic does the correct thing if the login is not taken, but a tombstone exists. See ebe6fb8 and
089fabc.

I made the decision to make no user-facing distinction and presenting it as before, as "login is already taken".

@lukasjuhrich
Copy link
Collaborator Author

Sorry for the abhorrent schema migration, but that's the way it is.

This is already covered in other tests
This is necessary to not break commands like `create-model`.
In prod, the flag should of course be set to `True`.
@lukasjuhrich
Copy link
Collaborator Author

Ignoring the pre-commit, for some reason darker broke and could not merge. Probably because the PR commit length is above the clone depth, so merge-base failed.

@lukasjuhrich lukasjuhrich merged commit f8f5d89 into develop Sep 9, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement more privacy-oriented unix account tombstoning with hashed login values
2 participants